Differentiation Dirichlet by matbesancon · Pull Request #1534 · JuliaStats/Distributions.jl

matbesancon · 2022-04-24T08:21:13Z

This adds the ChainRules rules for Dirichlet, constructor and logpdf

matbesancon · 2022-04-24T08:31:10Z

Some tests are performed with a manual finite diff with specific tangents that remain in the domain

devmotion

Thanks! I'll do a proper review later, here just two quick initial comments.

src/multivariate/dirichlet.jl

codecov-commenter · 2022-04-24T09:01:22Z

Codecov Report

Merging #1534 (7297260) into master (7c3af32) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1534      +/-   ##
==========================================
+ Coverage   85.74%   85.84%   +0.10%     
==========================================
  Files         129      129              
  Lines        7975     8018      +43     
==========================================
+ Hits         6838     6883      +45     
+ Misses       1137     1135       -2

Impacted Files	Coverage Δ
src/multivariate/dirichlet.jl	`77.91% <100.00%> (+5.09%)`	⬆️
src/show.jl	`100.00% <0.00%> (+2.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c3af32...7297260. Read the comment docs.

src/multivariate/dirichlet.jl

test/dirichlet.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

… into cr-dirichlet

matbesancon · 2022-04-25T17:50:49Z

On why _logpdf and not logpdf directly, it was to be matching the implementation signature.
We could add an rrule and frule for logpdf on all distributions that would just call into that of _logpdf?

matbesancon · 2022-04-25T18:03:56Z

For the tests, the rrule one fails even with forward finite diff, the finite diff version ends ups being NaN

src/multivariate/dirichlet.jl

devmotion · 2022-04-25T20:38:02Z

For the tests, the rrule one fails even with forward finite diff, the finite diff version ends ups being NaN

I assume the problem with finite differencing in general is that perturbing x leads to values that are not in the support anymore which therefore yield infinite log probabilities.

src/multivariate/dirichlet.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* Simplify implementation and tests * Precompute `digamma(alpha0)` * Relax type signature

devmotion

Looks good to me, is there anything else you would like to include in this PR?

matbesancon · 2022-05-27T15:25:56Z

setting it as draft to not merge it yet, I have to sort out some things before

…cr-dirichlet

matbesancon · 2022-07-31T08:50:38Z

Merging as already discussed and approved

matbesancon · 2022-10-11T08:50:55Z

No all good on my side thanks a lot!

…

On Thu, May 26, 2022, 16:03 David Widmann ***@***.***> wrote: ***@***.**** approved this pull request. Looks good to me, is there anything else you would like to include in this PR? — Reply to this email directly, view it on GitHub <#1534 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2FDMT3K3DHUZCJ2KONZ7DVL7KIPANCNFSM5UF5ANPA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

matbesancon added 5 commits April 16, 2022 14:41

constructor frule

ae0a0ad

frule tested

d407bf2

rrule tests

d5a293a

logpdf test

f9de7b3

signature for conflict

3723789

matbesancon requested a review from devmotion April 24, 2022 08:21

matbesancon added 2 commits April 24, 2022 10:21

TestUtils out of Project

5e32f04

ChainRules itself not needed (yet?)

0dde72f

devmotion reviewed Apr 24, 2022

View reviewed changes

src/multivariate/dirichlet.jl Outdated Show resolved Hide resolved

src/multivariate/dirichlet.jl Outdated Show resolved Hide resolved

remove checkarg

1348792

devmotion reviewed Apr 24, 2022

View reviewed changes

matbesancon and others added 8 commits April 25, 2022 08:55

Update src/multivariate/dirichlet.jl

90455c8

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update test/dirichlet.jl

25a41f3

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update test/dirichlet.jl

89a9346

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update test/dirichlet.jl

96883e8

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update src/multivariate/dirichlet.jl

ab01122

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

conflict

1d79fec

Merge branch 'cr-dirichlet' of github.com:JuliaStats/Distributions.jl…

bc29c40

… into cr-dirichlet

eltype instability

4cc7509

matbesancon added 2 commits April 25, 2022 19:58

single loop

0500772

fix tests

d2f832b

matbesancon added 2 commits April 25, 2022 22:08

forward finite diff

77ccee6

switch to broadcast

feafacd

devmotion reviewed Apr 25, 2022

View reviewed changes

src/multivariate/dirichlet.jl Outdated Show resolved Hide resolved

matbesancon added 2 commits April 25, 2022 22:55

fix broadcast

1f06aa6

switch off-support value to NaN

e702017

devmotion reviewed Apr 28, 2022

View reviewed changes

matbesancon and others added 5 commits April 29, 2022 09:37

Update src/multivariate/dirichlet.jl

475a934

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

Update src/multivariate/dirichlet.jl

7515e86

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

do not assume inplace

1a3fdd9

conflict

76cc96a

fixed temp

cb4f07e

devmotion mentioned this pull request May 23, 2022

Simplify implementation and tests in #1534 #1555

Merged

devmotion and others added 2 commits May 24, 2022 15:28

Simplify implementation and tests in #1534 (#1555)

9234155

* Simplify implementation and tests * Precompute `digamma(alpha0)` * Relax type signature

conflict

2c7100e

devmotion approved these changes May 26, 2022

View reviewed changes

matbesancon marked this pull request as draft May 27, 2022 15:25

Merge branch 'master' of github.com:JuliaStats/Distributions.jl into …

7297260

…cr-dirichlet

matbesancon marked this pull request as ready for review July 31, 2022 07:11

matbesancon merged commit b957872 into master Jul 31, 2022

matbesancon deleted the cr-dirichlet branch July 31, 2022 08:50

Conversation

matbesancon commented Apr 24, 2022

Uh oh!

matbesancon commented Apr 24, 2022

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matbesancon commented Apr 25, 2022

Uh oh!

matbesancon commented Apr 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devmotion commented Apr 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devmotion left a comment

Choose a reason for hiding this comment

Uh oh!

matbesancon commented May 27, 2022

Uh oh!

matbesancon commented Jul 31, 2022

Uh oh!

matbesancon commented Oct 11, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Apr 24, 2022 •

edited

Loading